Skip to content

chore: use testify's require in tests - #2792

Open
Tofel wants to merge 3 commits into
dx-5122-alerts-assertion-p12from
dx-5122-alerts-assertion-p13
Open

chore: use testify's require in tests#2792
Tofel wants to merge 3 commits into
dx-5122-alerts-assertion-p12from
dx-5122-alerts-assertion-p13

Conversation

@Tofel

@Tofel Tofel commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Migrates all test assertions from hand-rolled t.Fatal/Error to testify require, adds the testify dependency, and adds a recorder-mode fail-fast in check.go for from < StartedAt.

Review focus: the check.go fail-fast (the only non-test change); the rest is a mechanical assertion swap.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

📊 API Diff Results

No changes detected for module github.com/smartcontractkit/chainlink-testing-framework/grafana-alertcheck

View full report

@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p13 branch from c97e92d to 10c74ca Compare September 4, 2026 15:03
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p13 branch from 10c74ca to 33eea1f Compare September 4, 2026 15:15
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p13 branch from 33eea1f to a0a237e Compare September 7, 2026 09:35
@Tofel
Tofel marked this pull request as ready for review September 7, 2026 09:38
@Tofel
Tofel requested a review from a team as a code owner September 7, 2026 09:38
Copilot AI lite review requested due to automatic review settings September 7, 2026 09:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new recorder-mode fail-fast can mask log-identity failures, and a couple of updated tests use require.Fail from goroutines via observeAll, which is unreliable.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR standardizes the Grafana alertcheck test suite on testify/require for fail-fast assertions, adds the testify module dependency, and introduces a recorder-mode “fail fast” in internal/gate/check.go when from is before the recorded header’s StartedAt.

Changes:

  • Replace hand-rolled t.Fatal/t.Error patterns across tests with require.* assertions.
  • Add github.com/stretchr/testify (and indirect YAML dep) to go.mod/go.sum.
  • Add a recorder-mode early exit in check.go for the statically-detectable from < StartedAt bound violation.
File summaries
File Description
grafana-alertcheck/internal/gate/watch_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/watch_daemon_test.go Mechanical migration of assertions to require in integration-style daemon tests.
grafana-alertcheck/internal/gate/source_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/schedule_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/resolve_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/parse_state_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/parse_ruler_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/log_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/jsonreq_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/duration_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/coverage_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/classify_test.go Mechanical migration of assertions to require.
grafana-alertcheck/internal/gate/check.go Adds recorder-mode fail-fast when from precedes StartedAt.
grafana-alertcheck/internal/gate/check_test.go Updates tests to require and adds coverage for the new fail-fast path.
grafana-alertcheck/go.mod Adds testify dependency (and indirect YAML dep).
grafana-alertcheck/go.sum Adds checksums for newly introduced dependencies.
grafana-alertcheck/cmd/grafana-alertcheck/watch_test.go Mechanical migration of CLI tests to require.
grafana-alertcheck/cmd/grafana-alertcheck/table_test.go Mechanical migration of table rendering tests to require.
grafana-alertcheck/cmd/grafana-alertcheck/main_test.go Mechanical migration of CLI entrypoint tests to require.
grafana-alertcheck/cmd/grafana-alertcheck/list_test.go Mechanical migration of CLI list tests to require.
grafana-alertcheck/cmd/grafana-alertcheck/check_test.go Mechanical migration of CLI check tests to require.
Review details

Suppressed comments (1)

grafana-alertcheck/internal/gate/check_test.go:895

  • This responder can be invoked from goroutines via observeAll during the drain wait. require.Fail triggers FailNow from a worker goroutine, which doesn't reliably stop the test. Prefer returning an error (the caller path already makes the test fail by violating the require.NoError assertion).
		src := newCheckSource(func(title string, _ int) (Observation, error) {
			require.Fail(t, fmt.Sprintf("the drain wait polled skipped rule %q", title))
			return Observation{}, errors.New("unexpected poll")
		})
  • Files reviewed: 20/21 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread grafana-alertcheck/internal/gate/check.go Outdated
Comment thread grafana-alertcheck/internal/gate/check_test.go
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p13 branch 2 times, most recently from 4855fcd to a87fc14 Compare September 7, 2026 15:02
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p13 branch from a87fc14 to ca3ccc6 Compare September 7, 2026 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants